Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preliminary proof-of-concept for whereContainsIn #4

Merged
merged 7 commits into from
Mar 11, 2024

Conversation

blnkt
Copy link

@blnkt blnkt commented Feb 22, 2024

EPO-8705 Preliminary proof-of-concept for whereContainsIn

#390

@blnkt blnkt marked this pull request as ready for review February 22, 2024 20:13
@blnkt blnkt assigned ericdrosas87 and blnkt and unassigned ericdrosas87 and blnkt Feb 22, 2024
@blnkt
Copy link
Author

blnkt commented Feb 22, 2024

Change Requests

  • If more than one key is provided it seems to only check the first key. For instance, given imageAlbum(whereContainsIn: {keys: ["smartTags"], value: "Board"}) returns a collection imageAlbum(whereContainsIn: {keys: ["name", "smartTags"], value: "Board"}) returns an empty array (presumably because no name includes "Board")
  • Referring to the discovery issue, item 5 in my comment had a requirement of treating each word in the value as a term to try to match on. That functionality does not seem to be present yet. @ericdrosas87 decides whether it is the prerogative of the method args, or the method to enable this functionality (i.e. value can be a list of strings if you want, or your method can take the string it gets and split it into an array on whitespace).

@khalwat
Copy link
Collaborator

khalwat commented Feb 22, 2024

I didn't look deeply at the code, but the approach is exactly how I'd do it. 👍

@blnkt
Copy link
Author

blnkt commented Feb 28, 2024

The second part of my previous request (breaking search term into words each of which is used to find matches) seems to be working as expected, but the first part would still seem to be an issue. Attached find a screengrab showing that the order of the keys seems to have a direct impact on what results are returned (i.e. different results for different order) AND that the order of the words in the search does NOT seem to have a direct impact on what results are returned (as expected).

Screen.Recording.2024-02-28.at.12.32.51.PM.mov

@ericdrosas87
Copy link
Contributor

The second part of my previous request (breaking search term into words each of which is used to find matches) seems to be working as expected, but the first part would still seem to be an issue. Attached find a screengrab showing that the order of the keys seems to have a direct impact on what results are returned (i.e. different results for different order)

Interesting, I'll take a look at why that first issue is happening. What Canto album are you using?

AND that the order of the words in the search does NOT seem to have a direct impact on what results are returned (as expected)

We never talked about the ordering of the words in the search terms, I don't know where you're getting this idea from?

@ericdrosas87
Copy link
Contributor

ericdrosas87 commented Feb 28, 2024

Wait I'm not understanding what you're trying to show in that screen capture video, those are two completely different queries so returning different results would be expected?

Edit: Nevermind now I see what you're saying, it's returning different results based on the ordering of the keys, I'll look into this

Fixed issue w/merge() not modifying arr in place & instead returning
Copy link
Author

@blnkt blnkt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working as expected. @ericdrosas87 please merge and cut a new release that includes this improvement.

@ericdrosas87 ericdrosas87 merged commit aa65de0 into develop-v4 Mar 11, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants